Skip to content

Config Management: Updates#42

Merged
AkhileshNegi merged 13 commits intomainfrom
feat/guardrail-config-management-integration
Feb 16, 2026
Merged

Config Management: Updates#42
AkhileshNegi merged 13 commits intomainfrom
feat/guardrail-config-management-integration

Conversation

@rkritika1508
Copy link
Copy Markdown
Collaborator

@rkritika1508 rkritika1508 commented Feb 11, 2026

Summary

Target issue is #48
Explain the motivation for making this change. What existing problem does the pull request solve?
We have to integrate config management changes between kaapi_guardrails and kaapi_backend. This will allow us to control which org / project uses which validators and run guardrails and perform other operations.

This PR is making the following changes -

  1. Updated the list_validators API to also accept a list of ids (for each validator config). So, this API will list the validators for the given ids.
  2. There's a schema difference between validator_config which contains validators for the given project and what is expected while running the guardrails API. So, we have a method that accepts validator payloads and transforms them.

Checklist

Before submitting a pull request, please ensure that you mark these task.

  • Ran fastapi run --reload app/main.py or docker compose up in the repository root and test.
  • If you've fixed a bug or added code that is tested and has test cases.

Notes

Please add here if any other information is required for the reviewer.

Summary by CodeRabbit

  • New Features

    • Added ability to filter validators by ID when listing validators using an optional query parameter
    • Response now includes rephrase_needed and safe_text fields
  • Improvements

    • on_fail_action field now defaults to "Fix"
  • Tests

    • Added integration tests for validator filtering and validation

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 11, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR extends validator configuration functionality by adding database indexes for performance optimization, implementing ID-based filtering in the validator list endpoint, introducing schema validation with field normalization, and setting a default value for the on_fail_action field.

Changes

Cohort / File(s) Summary
Database Migration
backend/app/alembic/versions/003_added_validator_config.py
Added two new indexes on validator_config table: idx_validator_on_fail_action on on_fail_action column and idx_validator_is_enabled on is_enabled column. Downgrade properly reverses changes by dropping new indexes first.
API Route and CRUD Layer
backend/app/api/routes/validator_configs.py, backend/app/crud/validator_config.py
Extended list_validators endpoint with optional ids query parameter for filtering validators by ID. Updated ValidatorConfigCrud.list method signature to accept and apply id-based filtering when provided. Return type updated to List[dict].
Schema Updates
backend/app/schemas/guardrail_config.py, backend/app/schemas/validator_config.py
Added model_validator hook normalize_validators_from_config_api to GuardrailRequest for field normalization. Added rephrase_needed and safe_text fields to GuardrailResponse. Changed ValidatorBase.on_fail_action from required to optional with default value GuardrailOnFail.Fix.
Integration Tests
backend/app/tests/test_validator_configs_integration.py
Added comprehensive tests for id-based filtering (single and multiple IDs), invalid UUID validation (422 response), and invalid path parameter validation for get validator endpoint.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

enhancement, ready-for-review

Suggested reviewers

  • nishika26
  • AkhileshNegi

Poem

🐰 Indexes hop through the database floor,
Filtering validators like never before!
With ID-based searches and schemas so clean,
The finest validator configs you've seen! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'Config Management: Updates' is vague and overly generic, using non-descriptive language that doesn't convey meaningful information about the specific changes in the changeset. Use a more specific title that highlights the primary change, such as 'Add IDs filter to validator list API and config normalization for guardrail integration'.
✅ Passed checks (2 passed)
Check name Status Explanation
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/guardrail-config-management-integration

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@rkritika1508 rkritika1508 self-assigned this Feb 12, 2026
@rkritika1508 rkritika1508 marked this pull request as ready for review February 12, 2026 18:52
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
backend/app/api/routes/validator_configs.py (1)

67-90: ⚠️ Potential issue | 🔴 Critical

Route shadowing: GET /{config_id} shadows GET /{id} — the single-get endpoint is unreachable.

Both endpoints are GET on /{uuid}. FastAPI matches routes in declaration order, so list_validators_by_config_id (Line 67) will capture every request, and get_validator (Line 81) will never be reached.

You need to disambiguate these routes. For example, use a distinct path prefix for one of them:

Proposed fix — use a sub-path for config_id listing
-@router.get("/{config_id}", response_model=APIResponse[list[ValidatorResponse]])
+@router.get("/by-config/{config_id}", response_model=APIResponse[list[ValidatorResponse]])
 def list_validators_by_config_id(
     config_id: UUID,
backend/app/tests/test_validator_configs_integration.py (2)

95-97: ⚠️ Potential issue | 🟡 Minor

Inconsistent trailing slash in delete_validator URL.

delete_validator inserts a / between validator_id and the query string ({validator_id}/{DEFAULT_QUERY_PARAMS}), while update_validator (line 92) and get_validator (line 69) do not. Most frameworks handle this via redirect, but it's inconsistent and could cause issues if trailing-slash redirects are disabled.

Proposed fix
     def delete_validator(self, client, validator_id):
         """Helper to delete a validator."""
-        return client.delete(f"{BASE_URL}{validator_id}/{DEFAULT_QUERY_PARAMS}")
+        return client.delete(f"{BASE_URL}{validator_id}{DEFAULT_QUERY_PARAMS}")

359-374: ⚠️ Potential issue | 🟠 Major

Bug: verification uses row id instead of config_id, making the "still exists" check ineffective.

On line 373, self.get_validator(integration_client, validator_id) passes the row-level id to an endpoint that now expects a config_id. The endpoint will return an empty list (status 200) regardless, so line 374's assert get_response.status_code == 200 will always pass—without actually confirming the validator still exists.

Proposed fix
     def test_delete_validator_wrong_org(self, integration_client, clear_database):
         """Test that deleting validator from different org returns 404."""
         # Create a validator for org 1
         create_response = self.create_validator(integration_client, "minimal")
         validator_id = create_response.json()["data"]["id"]
+        config_id = create_response.json()["data"]["config_id"]
 
         # Try to delete it as different org
         response = integration_client.delete(
             f"{BASE_URL}{validator_id}/?organization_id=2&project_id=1",
         )
 
         assert response.status_code == 404
 
         # Verify original is still there
-        get_response = self.get_validator(integration_client, validator_id)
+        get_response = self.get_validator(integration_client, config_id)
         assert get_response.status_code == 200
+        assert len(get_response.json()["data"]) == 1
🤖 Fix all issues with AI agents
In `@backend/app/crud/validator_config.py`:
- Around line 55-70: The try/except is swallowing errors and allowing a partial
batch to be committed; change the error handling in the block that builds
ValidatorConfig objects (the loop over payloads.validators using
split_validator_payload and creating ValidatorConfig instances into objs) so
that any exception logs via the module logger (not print) and aborts the DB
operation—either rollback the session and re-raise the exception or return an
error before calling session.commit(); ensure session.add_all(objs) and commit
only occur when the try block fully succeeds, and reference the same symbols
(payloads.validators, split_validator_payload, ValidatorConfig, objs,
session.add_all) when making the changes.
🧹 Nitpick comments (3)
backend/app/crud/validator_config.py (1)

1-1: Minor: typing.List is deprecated; use built-in list.

Per Ruff UP035. Lines 92 and 113 use List[dict] — replace with list[dict] for consistency with the rest of the file (e.g., Line 52).

backend/app/alembic/versions/003_added_validator_config.py (1)

58-61: Low-cardinality indexes on on_fail_action and is_enabled are unlikely to help.

These columns have very few distinct values (3 and 2 respectively). Single-column B-tree indexes on such columns provide negligible selectivity and add write overhead. Consider removing them unless they're part of specific composite query plans.

backend/app/tests/test_validator_configs_integration.py (1)

67-87: get_validator and list_validators_by_config_id are near-duplicates hitting the same endpoint.

Both methods construct the same URL pattern (BASE_URL + id + query_params). The only difference is the extra **query_params support in list_validators_by_config_id. Also, get_validator is now misleadingly named since the endpoint returns a list by config_id, not a single validator by row ID.

Consider consolidating into one method (e.g., list_validators_by_config_id) and updating all call sites, or at minimum have get_validator delegate to list_validators_by_config_id.

♻️ Proposed consolidation
-    def get_validator(self, client, validator_id):
-        """Helper to get a specific validator."""
-        return client.get(f"{BASE_URL}{validator_id}{DEFAULT_QUERY_PARAMS}")
-
     def list_validators(self, client, **query_params):
         """Helper to list validators with optional filters."""
         params_str = (
@@ -80,11 +76,11 @@
 
     def list_validators_by_config_id(self, client, config_id, **query_params):
-        """Helper to list validators by config_id."""
+        """Helper to list validators by config_id (also used for single-validator retrieval)."""
         params_str = (
             f"?organization_id={TEST_ORGANIZATION_ID}&project_id={TEST_PROJECT_ID}"
         )
         if query_params:
             params_str += "&" + "&".join(f"{k}={v}" for k, v in query_params.items())
         return client.get(f"{BASE_URL}{config_id}{params_str}")

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@backend/app/crud/validator_config.py`:
- Around line 117-146: The list_by_batch_items function currently omits
requested ValidatorBatchFetchItem entries that aren't found (using
flattened_rows.get and skipping if maybe_row is falsy), which hides missing
validators; update list_by_batch_items to surface unmatched items by collecting
missing entries (compare payload items to flattened_rows keys) and either log a
warning via the module logger with details (validator_config IDs and
validator_type) or extend the return to include metadata (e.g., {"found": [...],
"missing": [...]}) so callers can detect drift; reference the function
list_by_batch_items, the payload/ValidatorBatchFetchItem objects,
flattened_rows, maybe_row and response when implementing the change.
🧹 Nitpick comments (4)
backend/app/tests/test_guardrails_api.py (1)

5-6: Unused imports: GuardrailOnFail and GuardrailRequest are not referenced in any test.

These imports appear to have been added in preparation for new tests that aren't included yet. If tests exercising the normalizer and on-fail behavior are planned, consider adding them in this PR; otherwise these will be flagged by linters.

backend/app/alembic/versions/003_added_validator_config.py (1)

55-58: Indexes on low-cardinality columns may not be beneficial.

on_fail_action has only 3 possible enum values and is_enabled is a boolean. Standalone B-tree indexes on such low-cardinality columns rarely improve query performance and add write overhead. These are typically only useful as part of a composite index or if the value distribution is heavily skewed (e.g., very few disabled rows).

Consider whether these indexes are actually needed for your query patterns, or if a composite index would serve better.

backend/app/tests/test_validator_configs_integration.py (1)

89-147: Batch endpoint tests cover the happy path — consider adding error-case coverage.

The tests validate successful batch create and batch fetch, which is good. Consider adding tests for edge cases in a follow-up:

  • Batch create with duplicate type+stage combinations (should trigger IntegrityError / 400).
  • Batch fetch with non-existent IDs (should return partial or empty results).
  • Empty validators list in batch create.
backend/app/crud/validator_config.py (1)

2-2: Use built-in list instead of typing.List.

typing.List is deprecated since Python 3.9. Use list directly for type hints, which is already done elsewhere in this file (e.g., line 58).

Proposed fix
-from typing import Any, List, Optional
+from typing import Any, Optional

Then replace List[dict] with list[dict] and List[ValidatorBatchFetchItem] with list[ValidatorBatchFetchItem] on lines 102, 122, 123, 140.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
backend/app/schemas/validator_config.py (1)

23-24: Consider adding a minimum-length constraint on validators.

An empty list is currently valid, which would result in a no-op batch creation (committing zero rows). A min_length=1 constraint would give the caller an immediate 422 instead of a silent success with an empty result.

Proposed fix
+from pydantic import ConfigDict, field_validator
+
 class ValidatorBatchCreate(SQLModel):
-    validators: list[ValidatorCreate]
+    validators: list[ValidatorCreate] = Field(min_length=1)

Or using Pydantic's annotated form:

validators: list[ValidatorCreate] = Field(min_length=1)
backend/app/crud/validator_config.py (2)

2-2: Use list instead of typing.List (deprecated since Python 3.9).

typing.List is used on lines 102, 122, 123, and 137 while list (lowercase) is already used elsewhere in the file (e.g., line 58). Standardize to the built-in list for consistency and to clear the Ruff UP035 warning.

Proposed fix
-from typing import Any, List, Optional
+from typing import Any, Optional
-    ) -> List[dict]:
+    ) -> list[dict]:
-        payload: List[ValidatorBatchFetchItem],
-    ) -> List[dict]:
+        payload: list[ValidatorBatchFetchItem],
+    ) -> list[dict]:
-        response: List[dict] = []
+        response: list[dict] = []

Also applies to: 102-102, 122-123, 137-137


196-199: Config keys could shadow base model fields in flatten.

{**base, **config} lets config keys overwrite base fields (e.g., id, type, stage). While split_validator_payload prevents this at creation time, data inserted via migrations or direct DB access could contain conflicting keys. Consider giving base fields precedence or logging on collision.

Defensive option: base wins
-        return {**base, **config}
+        return {**config, **base}

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (5)
backend/app/alembic/versions/003_added_validator_config.py (1)

55-58: Low-cardinality indexes may not provide query benefits.

on_fail_action has only 3 enum values and is_enabled is a boolean. Single-column indexes on such low-cardinality columns rarely improve query performance and add write overhead. These columns are typically more useful as part of composite indexes (e.g., combined with organization_id + project_id) or as partial indexes (e.g., WHERE is_enabled = true).

Consider whether these standalone indexes are actually needed for your query patterns.

backend/app/api/routes/validator_configs.py (2)

9-9: Unused import: ValidatorBatchCreate.

ValidatorBatchCreate is imported but not used anywhere in this file.

Proposed fix
 from app.schemas.validator_config import (
-    ValidatorBatchCreate,
     ValidatorBatchFetchItem,
     ValidatorCreate,
     ValidatorResponse,
     ValidatorUpdate,
 )

54-65: Consider adding a batch size limit to prevent abuse.

The endpoint accepts an unbounded list[ValidatorBatchFetchItem]. A very large payload could generate a massive IN (...) clause. Consider adding a reasonable upper bound (e.g., 100 or 500 items) and returning a 400 if exceeded.

backend/app/crud/validator_config.py (2)

11-15: Unused import: ValidatorBatchCreate.

ValidatorBatchCreate is imported but not used in this file.

Proposed fix
 from app.schemas.validator_config import (
-    ValidatorBatchCreate,
     ValidatorBatchFetchItem,
     ValidatorCreate,
 )

2-2: Use built-in list instead of typing.List.

Per Ruff UP035, typing.List is deprecated in favor of the built-in list type hint (Python 3.9+). This applies to all usages in this file (lines 60, 75, 81, 95).

Proposed fix
-from typing import Any, List, Optional
+from typing import Any, Optional

Then replace List[dict]list[dict] and List[ValidatorBatchFetchItem]list[ValidatorBatchFetchItem] throughout.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@backend/app/schemas/validator_config.py`:
- Around line 24-25: ValidatorBatchFetchItem is defined but not used; either
remove the class from validator_config.py and delete its imports from
crud/validator_config.py and api/routes/validator_configs.py, or if you intend a
batch endpoint, keep ValidatorBatchFetchItem and update the relevant functions
(e.g., batch fetch/create handlers in crud/validator_config and routes in
api/routes/validator_configs) to accept and use that type in their signatures
and request/response handling; pick one approach and make the code and imports
consistent.
🧹 Nitpick comments (1)
backend/app/crud/validator_config.py (1)

2-2: Use built-in list instead of typing.List.

typing.List is deprecated since Python 3.9. Line 60 should use list[dict] directly.

Proposed fix
-from typing import Any, List, Optional
+from typing import Any, Optional

And on line 60:

-    ) -> List[dict]:
+    ) -> list[dict]:

@rkritika1508
Copy link
Copy Markdown
Collaborator Author

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 16, 2026

✅ Actions performed

Full review triggered.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@backend/app/crud/validator_config.py`:
- Around line 63-64: The current conditional "if ids:" treats an empty list the
same as None and skips applying the filter, returning all validators; change the
check to an explicit None test (replace the "if ids:" guard with "if ids is not
None:") so that when ids is an empty list the code still calls
ValidatorConfig.id.in_(ids) on the query variable and returns an empty result
set as intended.
🧹 Nitpick comments (3)
backend/app/schemas/guardrail_config.py (1)

40-84: Model validator applies to all GuardrailRequest instantiations, not just config-API payloads.

The mode="before" validator fires on every GuardrailRequest construction — including the normal guardrails runtime path. While the logic is safe (it only strips fields that don't belong to the runtime schema and remaps on_fail_actionon_fail only when on_fail is absent), this coupling is worth being intentional about. If a future validator config introduces a field that collides with drop_fields (e.g. stage used in a different sense), it would be silently dropped.

Consider making the normalization an explicit classmethod (e.g. GuardrailRequest.from_config_api(data)) rather than an implicit pre-validator, so callers opt in.

backend/app/alembic/versions/003_added_validator_config.py (1)

55-58: Indexes on low-cardinality columns add write overhead with minimal query benefit.

on_fail_action has only 3 possible values and is_enabled is a boolean. Single-column B-tree indexes on such columns are rarely selective enough to be used by the query planner — queries already filter by organization_id + project_id first, which narrows the result set sufficiently.

Consider removing these indexes or, if you do need to query by these columns frequently, using a composite index (e.g., (organization_id, project_id, is_enabled)).

backend/app/crud/validator_config.py (1)

2-2: Use built-in list instead of typing.List (deprecated per Ruff UP035).

Proposed fix
-from typing import List, Optional
+from typing import Optional

Then on line 57:

-    ) -> List[dict]:
+    ) -> list[dict]:

Comment on lines +63 to +64
if ids:
query = query.where(ValidatorConfig.id.in_(ids))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

if ids: treats an empty list the same as None — potentially surprising.

if ids: is falsy for both None and []. If a caller passes an empty list of IDs (e.g., the config API has no validators configured), the filter is skipped and all validators for the org/project are returned instead of an empty set. Use an explicit None check:

Proposed fix
-        if ids:
+        if ids is not None:
             query = query.where(ValidatorConfig.id.in_(ids))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if ids:
query = query.where(ValidatorConfig.id.in_(ids))
if ids is not None:
query = query.where(ValidatorConfig.id.in_(ids))
🤖 Prompt for AI Agents
In `@backend/app/crud/validator_config.py` around lines 63 - 64, The current
conditional "if ids:" treats an empty list the same as None and skips applying
the filter, returning all validators; change the check to an explicit None test
(replace the "if ids:" guard with "if ids is not None:") so that when ids is an
empty list the code still calls ValidatorConfig.id.in_(ids) on the query
variable and returns an empty result set as intended.

@AkhileshNegi AkhileshNegi changed the title Added guardrail_config changes for config management integration Config Management: Updates Feb 16, 2026
@AkhileshNegi AkhileshNegi linked an issue Feb 16, 2026 that may be closed by this pull request
@AkhileshNegi AkhileshNegi merged commit 637ef2a into main Feb 16, 2026
2 checks passed
@AkhileshNegi AkhileshNegi deleted the feat/guardrail-config-management-integration branch February 16, 2026 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add config management changes

3 participants